[thrift_proxy/router] Add service name matching to router implementation#4130
Merged
zuercher merged 8 commits intoenvoyproxy:masterfrom Aug 15, 2018
Merged
Conversation
RouteMatch, as well as invert flag. - create new matching subclasses and construct based on proto definition Signed-off-by: Brian Ramos <brirams@users.noreply.github.com>
… for various cases/combinations Signed-off-by: Brian Ramos <brirams@users.noreply.github.com>
matching. - better proto documentation - more explicit integration test config Signed-off-by: Brian Ramos <brirams@users.noreply.github.com>
…NameRouteEntryImpl to ensure colon(':') is appended to service name, add some invert logic documentation
Signed-off-by: Brian Ramos <brirams@users.noreply.github.com>
…trap and pick the appropriate destination for each test Signed-off-by: Brian Ramos <brirams@users.noreply.github.com>
Signed-off-by: Brian Ramos <brirams@users.noreply.github.com>
Signed-off-by: Brian Ramos <brirams@users.noreply.github.com>
brirams
force-pushed
the
brirams/#5550/thrift_service_name_matching
branch
from
1264d0a to
ec5a143
Compare
zuercher
reviewed
Aug 14, 2018
Member
zuercher
left a comment
zuercher
left a comment
There was a problem hiding this comment.
Couple small things, but otherwise looks good!
| throw EnvoyException("Cannot have an empty service name with inversion enabled"); | ||
| } | ||
|
|
||
| if (!service_name.empty() && !StringUtil::endsWith(service_name.c_str(), ":")) { |
Member
There was a problem hiding this comment.
StringUtil::endsWith takes a const std::string&, so you can just pass service_name. (This is constructing a new temp std::string copy for the call.)
Contributor
Author
There was a problem hiding this comment.
ah -- I think this is a vestige of when I using the class member as an arg to this. will update.
|
|
||
| private: | ||
| const std::string method_name_; | ||
| bool invert_; |
|
|
||
| private: | ||
| std::string service_name_; | ||
| bool invert_; |
Member
There was a problem hiding this comment.
nit: both of these should be const. That said, it makes the constructor tricky so, I think I'm ok with leaving service_name_ non-const.
Signed-off-by: Brian Ramos <brirams@users.noreply.github.com>
brian-pane
reviewed
Aug 14, 2018
| // If specified, the route must exactly match the request method name. As a special case, an | ||
| // empty string matches any request method name. | ||
| string method = 1; | ||
| oneof match_specifier { |
Contributor
There was a problem hiding this comment.
This will allow Thrift requests to be routed based on either method name or service name. @fishcakez and @rgs1, do you think we'll need to be able to route based on the combination of both, e.g. service_name=X and method_name=Y?
Member
There was a problem hiding this comment.
FWIW, you can do this already with method_name=service:method
Contributor
There was a problem hiding this comment.
Cool, thanks. That solves my one concern.
zuercher
approved these changes
Aug 14, 2018
Member
zuercher
left a comment
zuercher
left a comment
There was a problem hiding this comment.
Looks good to me. Thanks for pitching in!
Contributor
Author
|
anytime! |
Member
|
@brian-pane do you have any other concerns? |
brian-pane
approved these changes
Aug 15, 2018
Contributor
brian-pane
left a comment
brian-pane
left a comment
There was a problem hiding this comment.
Thanks for the new feature!
Contributor
Author
|
🎉🎉🎉 |
rshriram
pushed a commit
to rshriram/envoy
that referenced
this pull request
Oct 30, 2018
…#1938) This is far from finished, but it reduces memory usage by ~10%. Pulling the following changes from github.com/envoyproxy/envoy: c1cc68d stats: refactoring MetricImpl without strings (envoyproxy#4190) 36809d8 fuzz: coverage profile generation instructions. (envoyproxy#4193) ba40cc9 upstream: rebuild cluster when health check config is changed (envoyproxy#4075) 05c0d52 build: use clang-6.0. (envoyproxy#4168) 01f403e thrift_proxy: introduce header transport (envoyproxy#4082) 564d256 tcp: allow connection pool callers to store protocol state (envoyproxy#4131) 3e1d643 configs: match latest API changes (envoyproxy#4185) bc6a10c Fix wrong mock function name. (envoyproxy#4187) e994c1c Bump yaml-cpp so it builds with Visual Studio 15.8 (envoyproxy#4182) 3d1325e Converting envoy configs to V2 (envoyproxy#2957) 8d0680f Add timestamp to HealthCheckEvent definition (envoyproxy#4119) 497efb9 server: handle non-EnvoyExceptions safely if thrown in constructor. (envoyproxy#4173) 6d6fafd config: strengthen validation for gRPC config sources. (envoyproxy#4171) 132302c fuzz: reduce log level when running under fuzz engine. (envoyproxy#4161) 7c04ac2 test: fix V6EmptyOptions in coverage with IPv6 enable (envoyproxy#4155) 1b2219b ci: remove deprecated bazel --batch option (envoyproxy#4166) 2db6a4c ci: update clang to version 6.0 in the Ubuntu build image. (envoyproxy#4157) 71152b7 ratelimit: Add ratelimit custom response headers (envoyproxy#4015) 3062874 ssl: make Ssl::Connection const everywhere (envoyproxy#4179) 706e262 Handle validation of non expiring tokens in jwt_authn filter (envoyproxy#4007) f06e958 fuzz: tag trivial fuzzers with no_fuzz. (envoyproxy#4156) 27fb1d3 thrift_proxy: add service name matching to router implementation (envoyproxy#4130) 8c189a5 Make over provisioning factor configurable (envoyproxy#4003) 6c08fb4 Making test less flaky (envoyproxy#4149) 592775b fuzz: bare bones HCM fuzzer. (envoyproxy#4118) For istio/istio#7912. Signed-off-by: Piotr Sikora <piotrsikora@google.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description:
Currently, the thrift router only supports method matching as a way to route thrift requests. This builds on that by adding the ability to specify a service name that is used when matching. This change updates the
RouteMatchproto definition to use aoneoffield to indicate what type of matching should be done, as well as aninvertflag that will allow for inverse matching rules.Additionally:
RouteEntryImplBaseimplementations check that inversion and wildcard matching are not enabled at the same time, as this would result in no matches for a routeRisk Level:
Low
Testing: